Skip to content

Conversation

@damienmarchal
Copy link
Contributor

Red alert on the CI ! Fix it now.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@damienmarchal damienmarchal added pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status to review To notify reviewers to review this pull-request labels Apr 14, 2021
@jnbrunet
Copy link
Contributor

@damienmarchal can you put origin/master instead?

@damienmarchal
Copy link
Contributor Author

[ci-build]

@damienmarchal damienmarchal added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 15, 2021
@damienmarchal damienmarchal merged commit 2436ff0 into sofa-framework:master Apr 15, 2021
@guparan
Copy link
Contributor

guparan commented Apr 15, 2021

You should not put a branch name in GIT_TAG.
Now SOFA master branch depends on SofaPython3. We don't want that.

@jnbrunet
Copy link
Contributor

Yeah but if you don't do it, than we need to manually update the commit number each time SofaPython3 upgrades. They should follow each others.

@guparan
Copy link
Contributor

guparan commented Apr 15, 2021

What is the problem with manually updating the pointer?
In my opinion it is much less a pain than having ALL pull-request going red after a SofaPython3 merge.

@jnbrunet
Copy link
Contributor

jnbrunet commented Apr 15, 2021

What is the problem with manually updating the pointer?

We will forget to do it, I'm sure of it.

In my opinion it is much less a pain than having ALL pull-request going red after a SofaPython3 merge.

How could that happen? No SOFA components depend on it, and SP3 has its own CI on github.

@guparan
Copy link
Contributor

guparan commented Apr 15, 2021

Because we build SofaPython3 on SOFA CI.
It's the starting point of all my work on ExternalProject update workflow that we have been discussing for 3 weeks.
We want to ease the pain of handling ExternalProject pointers while still building those external project on SOFA CI.
See #1961

@jnbrunet
Copy link
Contributor

jnbrunet commented Apr 15, 2021

Yes I know, and I'm not questioning your work nor the 3 weeks of discussions.

But, if you are going to automatically merge the related PR in SP3 to its master branch, than why hard code the commit number? You can automatically update it on the CI, but not actually commit it. Once it passes, no need to change the "origin/master" since it contains your automatically merged SP3 PR.

If you don't do it, then upgrades in SP3 (bug fixes, new features) that do not have a related PR in SOFA won't be taken into account unless we go do a PR in SOFA to update this pointer. I can guarantee you that nobody will think of doing it. Which means that fetching SP3 within SOFA has great chance to be pointing to an old version.

In fact, the reason I asked this to Damien is exactly because of this, SOFA fetched me an outdated version of SP3, which didn't contain the latest bug fix in SP3.

@guparan
Copy link
Contributor

guparan commented Apr 16, 2021

I'm always having a hard time designing this type of workflow...
I agree with you that it's way easier to have a branch than a commit in GIT_TAG.
What will happen though is that SOFA CI could be failing because of changes in SP3/master.

For instance, this will happen:
1. Bob opens a SOFA PR that breaks SP3
-> the SOFA PR fails on SOFA CI
2. Bob opens a SP3 PR to propose a fix to this future change in SOFA that will break SP3
-> the SP3 PR fails on SP3 CI (because it fixes something that is not broken yet)
3. Thanks to my work on ExternalProject, SOFA CI is able to test the SOFA PR with a custom version for SP3 (the version of the SP3 PR)
-> the SOFA PR succeeds on SOFA CI
4. Both PRs are merged (in any order)
5. All the other SOFA PRs are now failing on SOFA CI and must be updated with SOFA/master

EDIT: corrected version below

@jnbrunet
Copy link
Contributor

Yeah I see your issue now, thanks !

I think it could be resolved by changing (improving?) a little bit our current CI strategy. Correct me if I'm wrong, but right now the CI does the following for a PR # XXX into the branch Master:

  1. Checkout PR#XXX branch
  2. Build SOFA
  3. Run tests

I think a better approach should be:

  1. Checkout origin/master
  2. Merge PR #XXX branch
  3. Build SOFA
  4. Run tests

This might also prevent some weird issues that we had were the CI passes, we merge, and then the CI start breaking everywhere: The CI never actually tested the merged PR.

I think this would fix your issue 5.

What do you think?

@guparan
Copy link
Contributor

guparan commented Apr 16, 2021

Actually SOFA CI is already building a merge for PRs so I was wrong with my use case before. 😅

Here is a corrected version of what could happen:

  1. Bob opens a SOFA PR that breaks SP3
    -> the SOFA PR fails on SOFA CI
  2. Bob opens a SP3 PR to propose a fix to this future change in SOFA that will break SP3
    -> the SP3 PR fails on SP3 CI (because it fixes something that is not broken yet)
  3. Thanks to my work on ExternalProject, SOFA CI is able to test the SOFA PR with a custom version for SP3 (the version of the SP3 PR)
    -> the SOFA PR succeeds on SOFA CI
  4. SP3 PR is merged first (to ensure that SOFA/master always succeeds on SOFA CI)
  5. All the other SOFA PRs are now failing on SOFA CI and must wait for Bob's SOFA PR to be merged
  6. SOFA PR is merged
  7. All the other SOFA PRs are fixed

The problematic point is the duration of step 5.
How to make sure it is short? Is it a good idea to ask our devs to quickly merge 2 PRs from 2 projects? What about when it will be more complicated with 2, 3 or more external projects needing to be fixed?

@jnbrunet
Copy link
Contributor

jnbrunet commented Apr 16, 2021

Hey @guparan ,

I would merge both at the same time (automatically), the trigger being the SOFA merge:

  1. SP3 PR is merged first (to ensure that SOFA/master always succeeds on SOFA CI)
    SOFA PR is manually merged
  2. SP3 PR is merged automatically (we can setup a workflow for this, or if you have something ready on the CI)
  3. All the other SOFA PRs should be fine

Just to emphasis the problem with the alternative (hard-coded commit number instead of a branch):

  1. PRs in SP3 can be merged in the master branch before the correction SP3 PR is merged , which means that SOFA will point to an older version of SP3, example:
    a. Bob opens a SOFA PR that breaks SP3
    b. Bob opens a SP3 PR with the correction
    c. Paul comes in an open a SP3 PR
    d. Paul's PR is accepted and merged
    e. Paul opens a SOFA PR to update the commit number
    f. Paul's PR get accepted and merged
    g. Bob's PR get accepted and merged
    f. SOFA commit number is now pointing to Bob's SP3 branch number, which misses Paul's commit
  2. How do you deal with squash/rebase merges in SP3? Will your script use the merged commit number? Or the latest commit number of the SP3 PR's branch?
  3. How do you deal with release branches? Will we need to manually edit the commit number for each new commit in release branches?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants